-
Notifications
You must be signed in to change notification settings - Fork 450
[Enhancement][Tool] Tree-style pretty ASTPrinter #1468
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a TileLang AST printing feature: new pass-config key registration in C++, TileLang-side config enum, a conditional hook in the PreLowerSemanticCheck phase, and a visitor-based AST pretty-printer implementation in Python. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Phase as PreLowerSemanticCheck
participant Config as PassContext
participant Printer as ASTPrinter
participant Visitor as _ASTPrintVisitor
participant AST as Module/Stmt
Phase->>Config: should_enable_ast_print(pass_ctx)?
Config-->>Phase: TL_AST_PRINT_ENABLE true/false
rect rgb(200,230,201)
Note over Phase,Printer: when enabled
Phase->>Printer: ASTPrinter()(mod)
Printer->>Printer: print metadata (params, ret, buffers, attrs)
Printer->>Visitor: instantiate visitor
Visitor->>AST: traverse nodes
Visitor-->>Visitor: format tree nodes (connectors, indentation)
Visitor-->>Printer: return formatted lines
Printer-->>Phase: completed printing
end
Phase->>Phase: continue semantic checks
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
👋 Hi! Thank you for contributing to the TileLang project. Please remember to run We appreciate you taking this step! Our team will review your contribution, and we look forward to your awesome work! 🚀 |
8061862 to
286abb1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tilelang/analysis/ast_printer.py (3)
88-91: Unnecessaryindent.appendafter visiting.Line 90 appends to the indent list after visiting is complete, but the visitor is never used again and the function returns immediately. This appears to be leftover code that has no effect.
🔎 Proposed fix
visitor.print_stmt_brief(func.body, func_body_prefix) + visitor.indent.append(_normal_indent) visitor.visit_stmt(func.body) - visitor.indent.append(_normal_indent) return funcNote: If the intent was to add initial indentation for the body traversal, the append should happen before
visit_stmt, not after.
32-56: Consider using TIR node's known fields instead of class dict introspection.The current approach uses
stmt.__class__.__dict__.keys()to discover fields, which may miss inherited fields or include unexpected entries. For robustness, consider using TIR's known field accessors or a predefined field list per node type.However, for a debugging tool, this approach is acceptable as-is.
84-84: Optional: Prefix unused parameters with underscore to silence linter warnings.The
modandctxparameters are required by theprim_func_passsignature but are unused. You can prefix them with underscores to indicate intentional non-use.🔎 Proposed fix
- def pass_fn(func: PrimFunc, mod, ctx) -> PrimFunc: + def pass_fn(func: PrimFunc, _mod, _ctx) -> PrimFunc:
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/op/builtin.ccsrc/op/builtin.htilelang/analysis/ast_printer.pytilelang/engine/phase.pytilelang/transform/pass_config.py
🧰 Additional context used
🧬 Code graph analysis (1)
tilelang/engine/phase.py (2)
tilelang/transform/__init__.py (1)
get_pass_context(12-14)tilelang/analysis/ast_printer.py (1)
ASTPrinter(79-93)
🪛 Ruff (0.14.10)
tilelang/analysis/ast_printer.py
84-84: Unused function argument: mod
(ARG001)
84-84: Unused function argument: ctx
(ARG001)
🔇 Additional comments (6)
src/op/builtin.h (1)
58-58: LGTM!The new
kASTPrintEnableconstant is correctly defined and follows the established naming convention and placement of other pass configuration keys in this file.src/op/builtin.cc (1)
37-37: LGTM!The pass configuration option is correctly registered with the appropriate
Booltype, consistent with other similar enable/disable flags in this file.tilelang/analysis/ast_printer.py (1)
7-15: LGTM!The module-level constants for tree formatting are well-organized and clearly named. The line limit of 140 characters and the use of box-drawing characters (
├──,└──) provide a clean visual structure.tilelang/transform/pass_config.py (1)
87-88: LGTM!The new
TL_AST_PRINT_ENABLEconfiguration key is well-documented and correctly aligned with the C++ counterpart inbuiltin.h.tilelang/engine/phase.py (2)
66-69: LGTM!The
should_enable_ast_printfunction correctly follows the established pattern of other configuration helper functions in this file.
121-123: LGTM!The conditional AST printing is appropriately integrated into
PreLowerSemanticCheck, placed before the semantic validation checks. This allows developers to inspect the AST before any validation errors occur.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR enhances the AST printer in TileLang from a simple debugging tool to a formal tree-style visualization tool similar to the rich library. The feature can be enabled/disabled via pass configuration options and provides hierarchical, indented AST printing with tree connectors for better readability.
Key changes:
- Added pass configuration key
TL_AST_PRINT_ENABLEto control AST printing - Implemented a visitor-based AST printer with tree-style formatting using Unicode box-drawing characters
- Integrated the printer into the pre-lower semantic check phase when enabled
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tilelang/transform/pass_config.py | Adds the TL_AST_PRINT_ENABLE configuration key for enabling AST printing |
| tilelang/engine/phase.py | Adds should_enable_ast_print() helper function and conditionally invokes AST printer during pre-lower checks |
| tilelang/analysis/ast_printer.py | Complete rewrite of AST printer using visitor pattern with tree-style formatting, indentation management, and field traversal logic |
| src/op/builtin.h | Declares C++ constant for AST print enable configuration |
| src/op/builtin.cc | Registers the AST print enable pass configuration option in C++ |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (4)
tilelang/analysis/ast_printer.py (4)
7-14: Module constants are well-defined.The tree connector characters and indentation settings provide a clear, readable tree structure. The naming convention concern was already noted in a previous review.
32-43: Child field tracking may lose earlier child fields if multiple exist.If a statement class has multiple child fields (e.g., both
bodyandblock), only the last encountered one is retained inchild_field_name. However, this appears acceptable for TIR statements which typically have a single child field per node type.
79-79: Consider adding return type annotation.For better type safety, add the return type annotation as previously suggested.
99-99: Dead code: indent append after traversal.This line appends to the indent list after
visit_stmthas completed. The visitor has finished printing at this point, so this operation has no effect.🔎 Proposed fix
visitor.print_stmt_brief(func.body, func_body_prefix) visitor.visit_stmt(func.body) - visitor.indent.append(_normal_indent) return func
🧹 Nitpick comments (1)
tilelang/analysis/ast_printer.py (1)
28-30: String parsing relies on TVM's repr format.The
split(" ")(double space) parsing is tied to TVM's current repr formatting. If the format changes, this may break or produce unexpected output.Consider adding a fallback or documenting the expected format:
def print_stmt_brief(self, stmt: Stmt, prefix: str) -> None: stmt_repr = repr(stmt).splitlines()[0] # Extract first segment before double-space (TVM script format) stmt_script = stmt_repr.split(" ")[0].strip() if " " in stmt_repr else stmt_repr.strip() self.print_with_clip(prefix + f"{stmt.__class__.__name__}: " + stmt_script)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tilelang/analysis/ast_printer.py
🧰 Additional context used
🧬 Code graph analysis (1)
tilelang/analysis/ast_printer.py (2)
src/op/atomic_add.cc (2)
s(423-423)s(423-423)tilelang/analysis/nested_loop_checker.py (1)
pass_fn(115-117)
🪛 Ruff (0.14.10)
tilelang/analysis/ast_printer.py
93-93: Unused function argument: mod
(ARG001)
93-93: Unused function argument: ctx
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test for Python 3.12 with CUDA-12.8 (on self-hosted-nvidia)
- GitHub Check: Test for Python 3.12 with Metal (on macos-latest)
- GitHub Check: Test for Python 3.12 with Nightly-ROCm-7.1 (on self-hosted-amd)
🔇 Additional comments (3)
tilelang/analysis/ast_printer.py (3)
1-5: LGTM!Imports are appropriate for the visitor-based AST printer implementation.
68-76: SeqStmt handling is well-implemented.The special handling for sequence statements correctly iterates children with proper tree connectors and indentation management.
93-100: Pass function signature follows TVM convention.The
modandctxparameters are required by theprim_func_passinterface even when unused. The static analysis warning is a false positive for this standard pattern.
LeiWang1999
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Pretty awesome.
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| from tvm.tir.stmt_functor import ir_transform | ||
|
|
||
|
|
||
| _child_fields = ["body", "block", "seq"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Traverse missing statement children in AST printer
The new tree printer only recurses into fields listed in _child_fields (currently just body, block, seq). TIR statement nodes like IfThenElse use then_case/else_case, and Block can have init, which are statements but won’t be visited or expanded, so whole subtrees disappear from the tree output. This is a regression from the previous ir_transform traversal, and it can mislead debugging whenever kernels include conditionals or other statement children not named body/block/seq. Consider expanding the child field list or delegating traversal to PyStmtExprVisitor for all statement children.
Useful? React with 👍 / 👎.
Previously we add a trivial ASTPrinter for debugging purpose. This PR improves it to a formal tool in TileLang, which features:
rich.Usage: annotate the kernel you want to print with:
The gemm example will be printed like this:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.